Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid updating the block gap when it's unchanged #5540

Merged

Conversation

liuchengxu
Copy link
Contributor

@liuchengxu liuchengxu commented Sep 1, 2024

There are basically three commits in this PR. Since all these commits essentially have no logical changes, I packed them into one PR. Review by per-commit is recommended.

  • The first commit avoids unnecessarily updating the block gap storage when the value remains unchanged, as discovered when I worked on Why request block body for fast sync? #5406.
  • The second commit is purely about format string style changes but deletes ~10 lines of code, which slightly helps me look into this file :P
  • The third commit is added to avoid the unnecessary block gap update in BlockchainDb.

Previously, the block gap storage could be updated even when the gap is
the same as before. This patch fixes it by only updating the storage
when the gap is changed.
Pure tiny refactoring, no logical changes.

The benefit of consistent format strings is 10 lines of deletion.

The other style changes are added by the rust formatter.
@liuchengxu liuchengxu force-pushed the avoid-updating-block-gap-when-unchanged branch from 3485cf4 to f2a260b Compare September 1, 2024 06:59
Previously, the block gap state in `BlockchainDb` in unconditionally updated
whenever `try_commit_operation` is invoked, even if the state hadn't changed.
This commit refines the logic to update the block gap state only when a change
occurs, reducing unnecessary writes and improving efficiency.
Copy link
Contributor

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with this part of the codebase. Overall looks good, but I don't know if block gap can be updated in the db from other places.

@davxy can you have a look, please?

meta_keys::BLOCK_GAP,
&(start, end).encode(),
);
if start > end {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure the block gap can't be updated externally and returned from self.blockchain.meta.read()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm pretty sure that update_block_gap() is the only way to update the gap in meta and it's only invoked at the end of try_commit_operation().

Copy link
Member

@davxy davxy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

&(start, end).encode(),
);
}
block_gap_updated = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related nit: In below condition number > best_num + One::one() implies number >One::one().
So the second condition can be removed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already addressed by #5343, feel free to ignore

@davxy davxy added the T0-node This PR/Issue is related to the topic “node”. label Sep 4, 2024
@davxy davxy added this pull request to the merge queue Sep 4, 2024
Merged via the queue into paritytech:master with commit 9b28a54 Sep 4, 2024
185 of 187 checks passed
@liuchengxu liuchengxu deleted the avoid-updating-block-gap-when-unchanged branch September 17, 2024 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants